Fix sycl::queue leak in usm_ndarray get_queue()/get_device() - #3042
Fix sycl::queue leak in usm_ndarray get_queue()/get_device()#3042vlad-perevezentsev wants to merge 3 commits into
get_queue()/get_device()#3042Conversation
|
View rendered docs @ https://intelpython.github.io/dpnp/pull/3042/index.html |
|
Array API standard conformance tests for dpnp=0.21.0dev6=py314ha0e2e8e_8 ran successfully. |
|
No base build to compare — fix_queue_leak into master |
| // UsmNDArray_GetQueueRef_ returns an owning copy (DPCTLQueue_Copy, | ||
| // i.e. `new sycl::queue`); wrap it in a unique_ptr to avoid leaking a | ||
| // queue per call. | ||
| DPCTLSyclQueueRef QRef = api.UsmNDArray_GetQueueRef_(raw_ar); |
There was a problem hiding this comment.
QRef might be NULL on OOM inside DPCTLQueue_Copy, while below dereferencing of NULL is UB.
It seems legacy and can be done in the follow-up PR.
| // queue per call. | ||
| DPCTLSyclQueueRef QRef = api.UsmNDArray_GetQueueRef_(raw_ar); | ||
| return *(reinterpret_cast<sycl::queue *>(QRef)); | ||
| std::unique_ptr<sycl::queue> q_ptr{ |
There was a problem hiding this comment.
One point worth considering: freeing via delete (the unique_ptr default deleter) relies on an undocumented implementation detail rather than dpctl's documented ownership contract.
DPCTLSyclQueueRefis documented as an opaque pointer, andDPCTLQueue_Copyis annotated__dpctl_give, whose contract is: "the caller now owns the object … to free it, use it exactly once as a value for a__dpctl_takeargument" — i.e.DPCTLQueue_Delete.deletehappens to be equivalent today only becauseDPCTLQueue_Deleteis literallydelete reinterpret_cast<sycl::queue*>(QRef)and dpnp/dpctl share the same runtime/allocator. If dpctl ever changed how it allocates the copy (pool/custom allocator), rawdeletewould break whileDPCTLQueue_Deletewould not.
Non-blocking, but to honor the documented contract we could use a unique_ptr with a custom deleter.
There was a problem hiding this comment.
A cleaner option that honors the contract and avoids the link dependency: add a UsmNDArray_RemoveQueueRef capi export that binds DPCTLQueue_Delete, and route the free through the capsule function pointer — mirroring how UsmNDArray_GetQueueRef_ already works, so no consumer needs to link libDPCTLSyclInterface.
dpnp/tensor/_usmarray.pyx:
cdef api void UsmNDArray_RemoveQueueRef(c_dpctl.DPCTLSyclQueueRef QRef):
"""Delete a DPCTLSyclQueueRef previously returned by UsmNDArray_GetQueueRef"""
c_dpctl.DPCTLQueue_Delete(QRef) # safe on NULLdpnp/include/dpnp4pybind11.hpp — add the struct member / ctor-init / assignment mirroring UsmNDArray_GetQueueRef_, then:
DPCTLSyclQueueRef QRef = api.UsmNDArray_GetQueueRef_(raw_ar);
auto qref_deleter = [](sycl::queue *q) {
detail::dpnp_capi::get().UsmNDArray_RemoveQueueRef_(
reinterpret_cast<DPCTLSyclQueueRef>(q));
};
std::unique_ptr<sycl::queue, decltype(qref_deleter)> q_ptr{
reinterpret_cast<sycl::queue *>(QRef), qref_deleter};
return *q_ptr;This keeps allocation and deallocation on dpctl's side of the ABI and gives a symmetric Get/Remove pair (the capi test would be the natural place for a round-trip check).
That change is non-blocking for the PR — the current delete is provably equivalent under the supported build. So can be also done in the follow-up.
| PyUSMArrayObject *raw_ar = usm_array_ptr(); | ||
|
|
||
| auto const &api = detail::dpnp_capi::get(); | ||
| // UsmNDArray_GetQueueRef_ returns an owning copy (DPCTLQueue_Copy, |
There was a problem hiding this comment.
Can we use a shorter comment?
3 participants
This PR fixes a memory leak in the C++ accessors
usm_ndarray::get_queue()andget_device()indpnp/include/dpnp4pybind11.hppUsmNDArray_GetQueueRef_(dpnp'susm_ndarray.get_queue_ref()) returns an owning copy of the queue (it callsDPCTLQueue_Copyi.e. newsycl::queue)Both accessors dereferenced this pointer and copied the queue out but never freed the heap copy, leaking one
sycl::queueper call. Under queue churn this keepsqueue_implalive and prevents reuse of the associated Level Zero command lists / event pools.The fix takes ownership of the returned copy with
std::unique_ptr<sycl::queue>.dpctl needs no change because its
Memory_GetQueueRefreturns a borrowed ref, whereas dpnp'sUsmNDArray_GetQueueRefis documented to return a copy so it must be deleted.